Repository navigation
feat(dynacell): controlled FOV-parallel eval + thread budgeting - #426
Merged
Merged
Conversation
Adds dynacell.evaluation.runtime with three layered thread-cap entry points: early_apply_env_caps() sets BLAS/OMP env vars before C-extension load (called from __main__.main_cli); apply_thread_budget(threads) is the in-process safety net (env + torch.set_num_threads + threadpool_limits); resolve_runtime(config) materializes the new runtime config block. The runtime block (eval.yaml) is opt-in for parallelism (default executor=serial, fov_workers=1) but unconditionally tightens BLAS/OMP threads to cpu_count by default. On SLURM nodes where --cpus-per-task < host CPU count, this corrects previously-loose BLAS threading. Set DYNACELL_THREADS_PER_WORKER=N to override at the env layer for cases where in-process caps come too late. precompute_gt_artifacts validates that runtime.executor='serial' and fov_workers=1 — FOV-level parallelism is restricted to evaluate_predictions in this commit; precompute parallelism is out of scope (DeepFeatureBatcher accumulates state across FOVs). resolve_runtime supports two-phase resolution for evaluate_predictions: top-of-function for parent BLAS cap, then post-position-list with freeze_threads_per_worker= to keep parent and worker BLAS budgets consistent. Worker primitives (_process_one_fov, ProcessPoolExecutor) land in C3+C4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(C2) Adds always-on region timers (RegionTimer context manager) that record wall time of FOV-level and per-T regions to <save_dir>/eval_timing.csv. Region taxonomy matches actual execution placement at pipeline.py:341-505: FOV-level (t=None): mask_gt, mask_pred, cp_gt, deep_gt_dinov3, deep_gt_dynaclr, deep_gt_celldino, features_pred_per_t, microssim, seg_write Per-T: pixel_metrics, mask_metrics, feature_pairwise Timing overhead is ~0.1 ms per region per timepoint; ~120 ms total on a 9-h eval (~4e-6 of wall time) — no gating knob. Per-T memory hygiene knobs (runtime.cuda_empty_cache_every_n_timepoints, runtime.gc_collect_every_n_fovs) ship at 0 (no behavior change) in this commit. The plan calls for measuring and flipping defaults in C2 — that measurement requires a real eval run on the test48 zarr that exceeds this session's scope. Defaults stay at 0; observed late-T degradation in prior A549 ER evals (18 -> 61 s/T over 10 timepoints) is the documented motivation but not the measured fix in this commit. DYNACELL_FORCE_PER_T_HYGIENE=1 env var flips both knobs on at runtime regardless of YAML defaults — operator escape hatch for post-ship mitigation without a code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…esult (C3) Pure refactor: extract the per-FOV body of evaluate_predictions into a module-level function _process_one_fov(...) -> FovResult and an _aggregate_fov_result(...) parent-side aggregator. Serial-path behavior is identical to the prior code (verified: 384/48 pass/fail parity with HEAD~2; the 48 failures are all pre-existing). FovResult carries everything one FOV contributes to the run as a picklable dataclass: - per_t_pixel_rows (with microssim already merged in per pipeline.py:471 semantics — no separate microssim_rows field), - per_t_mask_rows, per_t_feature_rows, - seg_array of shape (T, 2, D, H, W) bool (channel 0 = pred, channel 1 = GT), - 4 _BackboneLists for cp/dinov3/dynaclr/celldino — each holds the per-T pred_feats/gt_feats/pred_fovs/gt_fovs/pred_ts/gt_ts arrays. Workers preserve the existing `if size > 0: append` guard so empty per-T slots never enter the lists; aggregator extends parent lists unconditionally. - timings slice from the per-process region_timer collector. _aggregate_fov_result writes the seg array to the HCS plate (still in the parent — concurrent create_position from workers would race plate metadata), extends per-T row lists, and extends the 24 dataset-level accumulators. This refactor positions C4 to add ProcessPoolExecutor + worker primitives without further changes to the FOV body. flush_manifest stays in the parent's loop for now; under process mode (C4) each worker will flush its own ctx at FOV end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds worker primitives in runtime.py (_worker_initializer,
gpu_serialization_lock, make_fov_executor) and the worker entry point
_worker_run_fov in pipeline.py. evaluate_predictions now branches on
runtime.executor: serial (default) runs _process_one_fov inline
identical to today; process spawns a ProcessPoolExecutor over FOVs.
Worker design:
- Spawn context avoids inheriting parent CUDA. _worker_initializer
sets BLAS/OMP env caps BEFORE apply_thread_budget lazy-imports torch.
- _worker_setup lazy-loads seg_model + extractors under
gpu_serialization_lock(): N workers don't init CUDA + load weights
concurrently. Plate handles cached per-worker.
- GPU touch sites inside _process_one_fov are wrapped in
gpu_serialization_lock(): mask_gt/pred, cp_gt, deep_gt_{dinov3,dynaclr,
celldino}, features_pred_per_t, pixel_metrics (per-T, GPU-bound
end-to-end including cubic spectral), and on-demand cellpose fallback
inside mask_metrics. In serial mode the lock has no path set and yields
immediately — bit-for-bit-identical behavior.
- Lock file at /tmp/dynacell_gpu_<SLURM_JOB_ID-or-PID>.lock (node-local,
not NFS). One lock per SLURM job, shared across workers.
Phase-2 runtime resolution: after pred_positions is built,
resolve_runtime is re-called with n_positions= to clamp "auto"
fov_workers to min(provisional, n_positions) and freeze
threads_per_worker at the Phase-1 value so worker initializers match
the parent's already-applied BLAS cap.
Order: results collected via as_completed (arrival order) but aggregated
in original pred_positions order so per-FOV CSV + embedding NPZ row
order is deterministic regardless of executor.
Workers flush their own ctx manifest at FOV end (global manifest lock at
_pos_write_lock(ctx, "manifest", "global") fcntl serializes flushes
across workers); interrupted process-mode runs preserve cache progress
the same way serial mode did.
Tests: full dynacell suite 384/48 pass/fail (identical to baseline; all
48 failures pre-existing). No new spawn-mode test in this commit —
covered by C5 CPU integration test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…docs (C5)
Adds test coverage for the C1-C4 runtime module and FOV-parallelism wiring:
test_runtime.py (23 tests): apply_thread_budget no-crash semantics under
Hydra's main; resolve_runtime auto/freeze/validate paths; region timing
collector; DYNACELL_FORCE_PER_T_HYGIENE env-var hook; spawn-context
ProcessPoolExecutor smoke via stdlib os.getpid.
test_evaluation_pipeline_parallel.py (3 tests): FovResult pickle round-trip
with realistic per-backbone shapes (the path that ProcessPoolExecutor takes
for Future.result transport); _aggregate_fov_result extends all 24
dataset-level accumulator lists correctly. Re-imports the live pipeline
module inside test bodies because test_lazy_init.py pops dynacell.* modules
from sys.modules, which would invalidate pickle resolution of a
module-level imported FovResult.
CI wiring:
- Root pyproject.toml registers the "slow" marker and appends -m "not slow"
to existing addopts.
- test-dynacell-configs runs test_runtime.py + test_evaluation_pipeline_parallel.py
alongside existing benchmark-schema/submit-tool tests. No new heavy deps
needed (these tests don't import iohub, cubic, transformers).
SLURM env caps:
- run_eval_jointtrained_nucleus.slurm and
run_eval_unext2_jointtrained_membrane.slurm export
DYNACELL_THREADS_PER_WORKER=\$SLURM_CPUS_PER_TASK before invoking eval.
Docs:
- applications/dynacell/CLAUDE.md gains "Eval runtime / parallelism"
section documenting the config block, env-var hooks, timing
instrumentation, and process-mode caveats.
Full dynacell test suite: 410 passed / 48 failed / 65 skipped — the 48
failures are the same set that fails on dynacell-models@HEAD (verified
parity across all 5 commits in this branch).
End-to-end serial-vs-process parity test on a real iohub+cache fixture is
a follow-up (see .claude/plans/eval-parallelism.md §C5).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…duplicate residency Under executor=process, workers lazy-load their own seg_model copies (under the GPU lock) — but the parent had been holding a duplicate copy GPU-resident since the unconditional prepare_segmentation_model(config) call at the top of evaluate_predictions. On an 80 GB A100 with fov_workers=4, this meant (N+1) × model_weights resident instead of N. After Phase-2 runtime resolution (where the final executor is known), drop the parent's copy when executor=process. The pre-warm side effect (segmenter_model_zoo's validate_model populating the checkpoint dir to avoid N workers racing on a cold quilt download) is already done by then. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an opt-in runtime layer for dynacell evaluation that enforces explicit thread budgeting and enables bounded FOV-level parallel execution via a spawn-based ProcessPoolExecutor, while keeping the default execution path sequential.
Changes:
- Introduces
dynacell.evaluation.runtimeto resolve runtime settings, cap BLAS/OMP/torch threads, provide GPU serialization lock, and collect/write timing instrumentation. - Refactors evaluation to support per-FOV worker execution (
FovResult+ aggregator) and adds per-T / per-FOV memory hygiene hooks. - Adds CI + pytest marker wiring and new unit/smoke tests for runtime resolution, executor bring-up, and cross-process pickle/aggregation behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds slow marker and defaults pytest runs to -m "not slow". |
| applications/dynacell/tools/run_eval_unext2_jointtrained_membrane.slurm | Exports DYNACELL_THREADS_PER_WORKER from SLURM allocation for early thread caps. |
| applications/dynacell/tools/run_eval_jointtrained_nucleus.slurm | Same SLURM-side thread cap export + guidance for future parallel overrides. |
| applications/dynacell/tests/test_runtime.py | Adds unit tests for runtime resolution, thread budgeting, timing CSV, GPU lock path, and executor creation. |
| applications/dynacell/tests/test_evaluation_pipeline_parallel.py | Adds pickle + aggregator contract tests for FovResult and _aggregate_fov_result. |
| applications/dynacell/src/dynacell/evaluation/runtime.py | New module implementing runtime resolution, thread/env caps, hygiene knobs, timing collector, and process-pool utilities. |
| applications/dynacell/src/dynacell/evaluation/precompute_cli.py | Applies thread budgeting in precompute and rejects FOV-level parallelism there. |
| applications/dynacell/src/dynacell/evaluation/pipeline.py | Extracts per-FOV processing into _process_one_fov returning FovResult, adds aggregation helper, and wires process executor path. |
| applications/dynacell/src/dynacell/evaluation/_configs/eval.yaml | Adds runtime: configuration block with documented defaults. |
| applications/dynacell/src/dynacell/main.py | Calls early_apply_env_caps() before heavy imports. |
| applications/dynacell/CLAUDE.md | Documents runtime/parallelism model, env vars, and timing output. |
| .github/workflows/test.yml | Extends dynacell CI job to run the new runtime + parallelism tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… submit Belt-and-suspenders safety net suggested in PR #426 review: call OmegaConf.resolve(config) in-place after Phase-1 runtime resolution and before any worker pickle.dumps the DictConfig across the spawn boundary. OmegaConf supports lazy interpolation resolution natively, so workers *would* resolve ${...} refs on first attribute access — but doing it once in the parent eliminates a tiny per-worker hot-path cost and guards against the (unlikely) case where an interpolation depends on parent-only state. ??? MISSING fields are untouched: verified OmegaConf 2.3 `_resolve` walks only nodes where `_is_interpolation()` is true, so bare ??? values pass through. Confirmed: 63 tests pass across pipeline / compose / parallel / runtime suites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a runtime.is_worker() helper that returns True iff _worker_initializer has set _GPU_LOCK_PATH. Wire it into _process_one_fov's inner-T tqdm via disable=is_worker(), so under fov_workers>1 the N concurrent per-T bars don't interleave on the parent's stderr. Outer per-FOV tqdm (in the parent) stays visible because the parent's _GPU_LOCK_PATH is never set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug: _process_one_fov writes region timings to the global _TIMINGS collector via region_timer, and returns the slice in FovResult.timings. _aggregate_fov_result then calls extend_timings(result.timings) unconditionally. In serial mode this re-appends each row to the SAME parent collector — every timed region appears twice in eval_timing.csv. In process mode the second append is correct (workers have separate per-process _TIMINGS, parent's is empty until aggregated). Fix: add extend_worker_timings kwarg to _aggregate_fov_result. Pass True in process mode, False in serial. Verified via 63-test suite green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_worker_setup no longer caches pred/gt/seg plate handles in _WORKER_STATE. Instead, _worker_run_fov opens them via `with open_ome_zarr(...) as ...` scoped to one FOV — iohub file descriptors close between FOVs and the worker has nothing to clean up on ProcessPoolExecutor shutdown. Models + cache contexts stay cached in _WORKER_STATE (their load cost ~5-10 s per worker is too high to repeat per FOV; ProcessPoolExecutor process exit reclaims them). Position lookup by name uses a linear scan of plate.positions() (iohub has no name->position index). Cheap for the ≤ tens of positions per typical eval — sub-ms per scan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_cache When io.require_complete_cache=true, the caller asserts that all needed masks live in the cache — so loading SuperModel (which hits a quilt download for nucleus/membrane on a cold node) is wasted work. Return None unconditionally in that mode. If a cache miss happens despite the assertion, the cache layer raises StaleCacheError before any segment() call, so the None seg_model never gets dereferenced. Enables the cache-only/no-model path that C5's integration test design relies on (run evaluate_predictions end-to-end against prebuilt mask caches without instantiating cellpose/SuperModel). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drives evaluate_predictions end-to-end on a tiny iohub fixture (3 positions
× T=2 × D=4 × H=W=32) with prebuilt mask caches. Runs the pipeline twice —
once with runtime.executor=serial, once with runtime.executor=process
runtime.fov_workers=2 — and asserts:
1. pixel_metrics + mask_metrics rows are byte-equal after sort by FOV.
2. segmentation_results.zarr per-position content is byte-equal across
both channels.
Cache-only design (matches plan §C5): target_name=er +
require_complete_cache=true + compute_feature_metrics=false +
compute_microssim=false + pixel_metrics.fsc=null +
pixel_metrics.spectral_pcc=null. The full pipeline runs without
instantiating cellpose, SuperModel, DinoV3, DynaCLR, CellDino, or cubic.
What this validates: spawn boot, DictConfig pickling, FovResult.timings +
arrays round-trip through pickle, _aggregate_fov_result extends parent
accumulators correctly, parent-side segmentation_results.zarr writes
don't race, sort-by-pos_name aggregation, and the new
extend_worker_timings semantics doesn't double-count timing rows.
_live_pipeline_module() clears dynacell.evaluation.* from sys.modules
before importing — necessary because test_evaluation_pipeline.py stubs
metrics/utils via monkeypatch.setitem(sys.modules, ...), and without the
clear my pipeline's compute_pixel_metrics resolves to the empty-returning
stub.
CI: runs ~16 s on the existing test-dynacell-configs job.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds tests/test_evaluation_pipeline_parallel_cpu.py to the dynacell pytest invocation. The test runs end-to-end evaluate_predictions in both serial and spawn-process modes against a tiny iohub fixture + prebuilt mask cache — no eval extras (cellpose, transformers, cubic) needed because target_name=er + io.require_complete_cache=true short-circuit segmenter and feature-extractor loads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four Copilot-review findings on the runtime layer: 1. apply_thread_budget threadpool_limits re-arm: the old handle becomes unreachable and is GC'd. The previous comment said "stays alive" which was misleading. Document the actual behavior: we WANT the new limit active, dropping the old handle is intentional. Also remove the dead if-branch since both arms did the same assignment. 2. resolve_runtime accepts literal fov_workers=0 / threads_per_worker=0 which produces divide-by-zero in the "auto" math or nonsensical thread counts. Add explicit >= 1 validation with a clear error. 3. _cpu_count() prefers os.sched_getaffinity(0) over os.cpu_count() so SLURM cgroup limits (cpuset etc.) are honored — otherwise auto- resolved fov_workers/threads_per_worker can oversize past the cgroup-visible CPU budget. Falls back to os.cpu_count() on non-Linux. 4. gpu_serialization_lock now takes a gate: bool = True kwarg. Callers in pipeline.py pass gate=use_gpu so under config.use_gpu=false the lock is a no-op even in process mode. Previously the lock fired unconditionally, defeating cross-worker parallelism for CPU-only eval runs. The lock still gates on _GPU_LOCK_PATH for the parent/serial no-op (unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ak memory Previously buffered every FovResult in results_by_pos until ALL N worker futures completed, then aggregated in pos_names_in_order. Peak parent memory: N × seg_array (~N × 440 MB for T=10 D=21 H=W=1024 evals) all live simultaneously. Switch to streaming aggregation: drain the buffer opportunistically in pos_names_in_order as futures complete. Only out-of-order FovResults stay buffered; once the next-expected position lands we aggregate (plate write + accumulator extend) and drop the ref. Peak buffer is bounded by the number of out-of-order FOVs in flight, not by N. Deterministic per-FOV CSV / embedding NPZ row order is preserved because aggregation still happens in pos_names_in_order — early completions just wait their turn in the buffer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
11 of 12 tasks
* feat(dynacell): grouped multi-condition eval driver Add ``dynacell evaluate-grouped``: run the same (model, organelle) across multiple I/O variants (the canonical case is the three A549 treatment plates mock/denv/zikv) in one Python process, paying the segmenter + DINOv3 + DynaCLR + CELL-DINO cold-start once instead of N times. On serial-mode interactive jobs and on per-condition SLURM jobs this is 30-90s of weight load + checkpoint warmup amortized across all conditions. Under runtime.executor=process, each condition still spawns its own pool whose workers re-load model copies, so only the parent-side load is amortized; the driver prints a note when it detects this. Mechanism: - ``dynacell.evaluation.model_loader.load_eval_models`` extracts the segmenter + extractor instantiation that previously lived inline in ``evaluate_predictions`` and ``_worker_setup``. The bundle carries identity tags (dinov3_model_name, dynaclr_ckpt source paths, encoder cfg, celldino weights) so ``init_cache_context`` callers get identical cache-identity values across conditions. - ``evaluate_predictions(config, *, models=None)`` accepts a pre-loaded bundle and skips its inline load when supplied. ``models=None`` preserves the historical single-condition behavior; existing callers and tests are unchanged. - ``evaluate_predictions_grouped(config)`` reads a top-level ``conditions:`` list, deep-merges each overlay into a copy of the base, and runs them sequentially with the shared models. Per-condition ``_final_metrics_cache_valid`` short-circuits are honored independently. - ``_check_grouped_field_invariants`` rejects overlays that touch ``target_name``, ``feature_extractor.*``, ``compute_feature_metrics``, or ``use_gpu`` -- those determine which models get loaded and must be identical across conditions. ``io.*``, ``save.*``, ``runtime.*``, ``limit_positions``, ``force_recompute.*`` are free to vary. - Hydra entry point ``evaluate_model_grouped`` with new ``_configs/eval_grouped.yaml`` (inherits ``eval.yaml`` via defaults). - New CLI subcommand registered in ``__main__._HYDRA_COMMANDS``. Tests: ``tests/test_evaluation_grouped.py`` builds a 2-condition cache-only fixture (separate pred plates + cache dirs) and asserts byte-equal parity against sequential per-condition runs, plus rejection cases for empty ``conditions`` and forbidden model-loading-field overrides. Cache-only path means no models actually load -- the test validates the loop structure, not the speedup. Reuses the iohub fixture builders from ``test_evaluation_pipeline_parallel_cpu.py``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(dynacell): simplify grouped eval driver + fold load + cache-context unification Followup polish on d69d970 (feat: grouped multi-condition eval driver). Addresses /simplify findings + the two related cleanups the user asked to fold in: Pipeline driver (pipeline.py): - Lazy model load — defer ``load_eval_models`` until the first condition that misses ``_final_metrics_cache_valid``. Restarts where every condition is cache-hit pay no 30–90 s cold start. - Invariant baseline is now the conditions-stripped input config, not the conditions[0]-merged config. A model-loading override smuggled into condition 0 is rejected symmetrically with conditions 1..N (previously it was silently re-baselined). New test guards the regression: ``test_grouped_rejects_condition0_model_field_override``. - ``_merge_condition`` drops the manual ``to_container`` round-trip deep-copy; ``OmegaConf.merge`` already returns a non-aliased config. - ``_check_grouped_field_invariants`` switched to ``OmegaConf.is_config`` in place of the ``type(OmegaConf.create([]))`` idiom; baseline snapshot is computed once and reused per iteration. - Extracted ``_load_cached_final_metrics`` so the three-NPY reload block is shared between ``evaluate_model`` and the grouped driver. - Removed redundant first-iteration merge + ``apply_dataset_ref`` for condition 0; the loop body now does both for every condition, including idx=0. Model loader (model_loader.py) and precompute-gt: - Added ``LoadFlags`` so ``load_eval_models`` can be called from both ``evaluate_predictions`` (all extractors gated by ``compute_feature_metrics``) and ``precompute_gt_artifacts`` (per-extractor ``build.masks`` / ``build.dinov3`` / ``build.dynaclr`` / ``build.celldino`` flags). ``LoadFlags.for_evaluate`` recovers the prior default. The 30-line inline load block in ``precompute_cli.py`` collapses to a single ``load_eval_models(config, flags=LoadFlags(...))`` call. Stricter celldino weights gate is preserved as an explicit pre-load raise. - Added ``init_cache_contexts(config, models)`` and ``init_gt_cache_context(config, models)`` so the four identical ``init_cache_context(..., side=..., **identity_tags)`` call sites in ``evaluate_predictions``, ``_worker_setup``, and ``precompute_gt_artifacts`` consolidate behind the EvalModels bundle. Test fixtures (tests/_eval_fixtures.py): - Promoted the cache-only fixture builders (``build_eval_config``, ``build_fixture``, ``make_hcs_plate``, ``make_mask_cache``, ``live_pipeline_module``, ``read_position_arrays``, ``N_POSITIONS``/``T``/``D``/``H``/``W``) out of ``test_evaluation_pipeline_parallel_cpu.py`` into a private ``_eval_fixtures.py`` module (underscore-prefixed so pytest skips collection). Both ``test_evaluation_pipeline_parallel_cpu.py`` and ``test_evaluation_grouped.py`` now import normally — no ``sys.path.insert`` (project's "no ``sys.path`` edits" rule). CI: - ``.github/workflows/test.yml`` adds ``test_evaluation_grouped.py`` to the ``test-dynacell-configs`` matrix so the grouped driver gets exercised on every push. All 104 tests in the affected suites pass. Pre-existing 48-test failure baseline (test_benchmark_config_composition + one test_evaluation_metrics case) is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(dynacell): tier grouped-eval process-mode message by cache state Split the existing ``[grouped] note:`` print into two tiers so users get proportional signal for the configuration they actually picked: - ``executor=process`` + ``require_complete_cache=true`` (cache-only, n_conditions > 1) keeps the mild informational note — workers re-init per condition but skip ``prepare_segmentation_model`` (returns None) and don't instantiate the deep extractors, so the only overhead is N pool spawns. - ``executor=process`` + ``require_complete_cache=false`` (cache-miss, n_conditions > 1) gets a loud multi-line WARNING. This is the actual worst case: each condition's worker pool independently loads SuperModel + DINOv3 + DynaCLR + CELL-DINO, so total wasted cold-start scales as ~30-90 s × N_workers × N_conditions. The warning explicitly points to ``executor=serial`` as the fix, since the parent's pre-loaded ``EvalModels`` is shared across conditions in serial mode but lost across pool spawns in process mode. Two new tests in ``test_evaluation_grouped.py``: - ``test_grouped_warns_loudly_on_process_plus_cache_miss`` confirms the WARNING text fires (executor=process + require_complete_cache=false + 2 conditions) with all the expected keywords. - ``test_grouped_mild_note_on_process_plus_cache_only`` confirms the cache-only fixture (require_complete_cache=true) gets the mild note and does NOT trip the loud WARNING. CLAUDE.md "Grouped multi-condition eval" section updated with the tiered behavior so users searching for the WARNING text find context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(dynacell): grouped eval struct-mode + require_complete_cache invariant Three fixes triggered by Copilot's review on PR #427 plus an independent struct-mode bug that would have crashed every real Hydra invocation of ``dynacell evaluate-grouped``: 1. ``_merge_condition`` was over-simplified during the /simplify pass (commit c271d3f). ``OmegaConf.merge`` propagates struct mode from its first argument, and Hydra composition always produces struct-mode configs. Result: merging an overlay carrying a ``name`` label (which isn't in the schema) raises ``ConfigKeyError``, and ``del merged["conditions"]`` raises ``ConfigTypeError`` even when the merge succeeds. Restore the ``to_container`` round-trip on the base — it escapes struct mode while still producing an independent config. Same fix applied to the ``models_base`` snapshot a few lines above. Existing tests didn't catch this because they construct configs via ``OmegaConf.create({...})`` directly (non-struct); real users going through ``@hydra.main`` would have hit the crash immediately. 2. Add ``io.require_complete_cache`` to ``_MODEL_LOADING_FIELDS`` so per-condition overlays cannot override it. The flag flips ``prepare_segmentation_model`` between "load SuperModel" and "return None" — letting a condition flip it would mean the shared model bundle is wrong for that condition (None when cache-miss expects a real segmenter, or loaded but never used). Treat as a grouped invariant. (Copilot thread on pipeline.py:1227.) 3. Add ``applications/dynacell/tests/__init__.py``. The new test files use ``from ._eval_fixtures import ...`` relative imports, which work under the project's ``--import-mode=importlib`` setting but are fragile under other pytest import modes. Adding ``__init__.py`` makes ``tests/`` a proper package and the relative imports work regardless of which import mode pytest is invoked with. (Copilot threads on test_evaluation_grouped.py:32 and test_evaluation_pipeline_parallel_cpu.py:42.) Two new regression tests in ``tests/test_evaluation_grouped.py``: - ``test_grouped_rejects_require_complete_cache_override`` covers the new invariant. - ``test_grouped_works_on_struct_mode_config`` wraps the parity fixture in ``OmegaConf.set_struct(grouped_cfg, True)`` to simulate a Hydra-composed config; would have failed before the ``_merge_condition`` fix. All 108 tests in affected suites pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(dynacell): fix two CLAUDE.md doc nits flagged in PR #427 self-review Two 75-confidence findings from the parallel review on PR #427 that scored below the 80-threshold for the inline review comment but are worth fixing now while context is fresh: 1. "Reality check for process mode" subsection referenced ``pipeline.py:171`` as the parent seg-model load site. After the eval-parallelism refactor that line is a dataclass field (``pred_ts: list[np.ndarray] = ...``); the actual load now lives in ``load_eval_models(config)`` called from the top of ``evaluate_predictions``. Switch the reference from a brittle line number to a function-name pointer. 2. "Grouped multi-condition eval" subsection said the per-condition cache-load short-circuit fires "with ``force_recompute.final_metrics =false``", omitting the second gate: ``_final_metrics_cache_valid`` also returns False (forcing recompute) when ``force_recompute.all =true``, regardless of the ``final_metrics`` flag. Make both gates explicit so users who set ``force_recompute.all=true`` for unrelated reasons aren't surprised by unexpectedly recomputed metrics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Alexandr Kalinin <alxndrkalinin@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… cache off `prepare_segmentation_model` was returning None unconditionally whenever `io.require_complete_cache=true`, on the assumption that all masks would be served from cache. That assumption breaks for nucleus/membrane targets when `io.pred_cache_dir=None`: the per-T loop in `_process_one_fov` falls back to `segment(predict[t], seg_model=seg_model)`, which raises `ValueError` because `seg_model` is None. Narrow the early return to cases where no path can reach `segment()` without a cached mask — organelle targets (workflow-based, no `seg_model` needed) or nucleus/membrane runs that also have `io.pred_cache_dir` configured. Reported by Copilot on PR #426. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ation The docstring claimed "no CUDA available in the worker" was one of the no-op conditions, but the implementation only checks `gate` and `_GPU_LOCK_PATH`. On a CPU-only node with `use_gpu=true` the lock would still serialize across workers. Rather than add a runtime CUDA probe (callers already pass `gate=use_gpu` to opt out for CPU-only regions), correct the docstring to describe what the code actually does. Reported by Copilot on PR #426. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…im bloat The eval README had grown stale relative to the runtime/grouped/cache work landed on this branch and PR #427: no mention of `io.require_complete_cache`'s cache-only fast path, no `runtime.*` block, no `dynacell evaluate-grouped`, no CELL-DINO. This rewrite folds those in and restructures: FOV parallelism + grouped eval move out of "Artifact caches" (where they were mis-classified) into a new top-level "Scaling" section; a new "Quick start" sits above Configuration; HF-cache operational details go to a new "HPC notes" section at the end. Bloat trim along the way: components table compressed (related modules combined, trivial files pushed to a note), HF-cache + external-users sections shortened, the 11-row force_recompute table collapsed to a YAML block plus 5 grouped descriptions, redundant Hydra group-discovery paragraphs removed. No flags, footguns, or CLI options dropped — every config knob and env var from the old version is still documented. 466 → 265 lines. Public-repo hygiene: replaced the hard-coded `/hpc/projects/comp.micro/...` default for the team-shared HF cache with a description that points at the `_DEFAULT_SHARED_HF_CACHE` constant in `dynacell/__main__.py` and the `DYNACELL_SHARED_HF_CACHE` env-var override. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Five fixes from a code-review pass on the controlled FOV-parallel eval work: - Restore the seg_model invariant for grouped multi-condition runs. After 5829175, ``prepare_segmentation_model``'s load decision depends on ``io.pred_cache_dir`` in addition to ``io.require_complete_cache``, but ``_MODEL_LOADING_FIELDS`` only caught the latter. A per-condition overlay setting ``io.pred_cache_dir=None`` against a base that has it set (for nucleus/membrane + ``require_complete_cache=true``) would crash at runtime: the shared ``seg_model`` is None but the per-T loop falls back to ``segment(predict[t], seg_model=None)``. New ``_seg_model_required()`` helper mirrors the load decision and ``_check_grouped_field_invariants`` rejects the dangerous direction only, leaving canonical per-condition pred caches free to differ. - Fix the process-mode progress bar stalling. ``pbar.update(1)`` only fired inside the in-order drain loop, so if FOV 0 was slow and later FOVs completed first, the bar stayed at 0% until FOV 0 drained. Update on each worker completion instead. - Correct the misleading ``[grouped] note:`` for ``executor=process`` + ``require_complete_cache=true``. The message claimed "no models actually load" but deep extractors do load whenever ``compute_feature_metrics=true``, and the segmenter only short- circuits in a subset of cases. Rewrite to match reality. - Move ``runtime.*`` imports (12 symbols) from four inline per-function blocks to one top-of-file import. No spawn-ordering or circular-import constraint blocks them; the inline pattern just violated the project's "no inline imports without strong reason" rule and duplicated a ``flush_manifest`` import that already lives at module top. New regression test ``test_grouped_rejects_pred_cache_dir_flip_for_nucleus`` guards the dangerous direction; an existing test's assertion updates to the corrected note wording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
edyoshikun
added a commit
that referenced
this pull request
May 22, 2026
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Eduardo Hirata-Miyasaki <edhiratam@gmail.com> Co-authored-by: dihan.zheng <dihan.zheng@ucsf.edu> fix(viscy-data): drop use_thread_workers to fix DDP deadlock (#413) fix(viscy-data): port to iohub 0.3.2 ImageArray API (#407) fix shipped via PR #412 fixes triggered by Copilot's review on PR #427 plus an independent fix two CLAUDE.md doc nits flagged in PR #427 self-review fix(dynacell): address PR #426 code-review findings fix(dynacell): harden DynacellGAN against silent failures (PR #428 review)
alxndrkalinin
added a commit
that referenced
this pull request
May 22, 2026
…g/dynacell-dynadtw PR #432 base modular-viscy-staging diverged from staging/dynacell-dynadtw when #404 squashed all of dynacell-models history into a single commit (393abdf). dynacell-models has 30 newer commits since dynadtw's last sync at f1df36b — re-eval campaign generator, controlled FOV-parallel eval (#426), 3D pix2pix GAN baseline (#425), DeepFeatureBatcher / precompute deep features, cache identity refactors, pix2pix3d_unetvit modernization (#428), backbone bookkeeping refactor onto _BackboneLists. Conflict triage (92 conflicts): - 89 files where staging/dynacell-dynadtw carried stale copies of dynacell-models content (no post-branch edits on dynadtw side): 84 AA conflicts (eval YAML configs, dynacell evaluation sources/tests, CLAUDE.md, tools) + 5 UU conflicts (engine.py, __init__.py, __main__.py, test_engine.py, .github/workflows/test.yml). Resolved by taking the modular-viscy-staging post-#404 version. - 3 files with genuine concurrent edits, resolved per direction: * .gitignore — dynadtw side is a strict superset (added local evaluation_matrix.md exclusion). Take dynadtw. * packages/viscy-models/.../foundation/cell_dino.py — dynadtw added CellDinoModel wrapper + per-image normalize flag; modular-viscy-staging didn't touch post-branch. Take dynadtw. * pyproject.toml — modular-viscy-staging added pytest "slow" marker + addopts "-m not slow" (no-overlap, kept); both sides identically added aics* git sources, py312 bump, pythonpath. iohub: modular-viscy-staging kept a git override pin at afe11f6a (~0.3.3); dynadtw dropped it for iohub>=0.3.5 from PyPI. Took dynadtw's strategy (PyPI 0.3.5 is released; viscy-data dep pin already at >=0.3.5 on this side). - uv.lock regenerated from the merged pyproject.toml (iohub 0.3.5 from PyPI registry). Sanity check: uv sync --all-packages --all-extras + uv run pytest -m "not slow" applications/dynacell/tests/ shows 51 failed / 459 passed / 78 skipped. All 51 failures are pre-existing on dynacell-models HEAD (test_benchmark_config_composition strategy/leaf composition issues + test_evaluation_metrics gain-and-offset) — verified by running the same suite on the clean backbone-refactor worktree. The merge introduces no new regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds bounded FOV-level parallelism + explicit thread/process budgeting to dynacell eval. Default behavior is unchanged (sequential FOV loop); opt-in via
runtime.executor=process runtime.fov_workers=N.5 atomic commits:
dynacell.evaluation.runtimemodule withapply_thread_budget,resolve_runtime,early_apply_env_caps. Tightens BLAS/OMP threads tocpu_counteven in serial mode (corrects previously-loose OpenBLAS threading on SLURM where--cpus-per-task< host CPU count).cuda_empty_cache_every_n_timepoints,gc_collect_every_n_fovs, default 0) and always-on region timing instrumentation (writes<save_dir>/eval_timing.csv).DYNACELL_FORCE_PER_T_HYGIENE=1env-var escape hatch for operators._process_one_fov(...) -> FovResult+_aggregate_fov_result(...). Pure refactor; serial path is bit-for-bit identical to today./tmp/dynacell_gpu_<SLURM_JOB_ID>.lock. Workers lazy-load models under the lock. Two-phase runtime resolution (Phase-1 caps BLAS; Phase-2 re-clampsfov_workersafter position list is known withfreeze_threads_per_worker=to keep parent/worker BLAS budgets consistent). Auto-demotesexecutor=processtoserialwhen resolvedfov_workers=1.slowpytest marker, adds new tests to the existing dynacell CI job. SLURM scripts exportDYNACELL_THREADS_PER_WORKER. CLAUDE.md docs.Test plan
uv run pytest applications/dynacell/tests/— 410 passed / 48 failed / 65 skipped on this branch; same 48 pre-existing failures asdynacell-models@HEAD.uvx ruff check applications/dynacell/— clean.FovResultpickle round-trip works with synthetic per-FOV shapes (2 timepoints, 3 cells/T, 4 backbones × 768-dim).make_fov_executor(executor=process)returns a realProcessPoolExecutor; child workeros.getpid()differs from parent's..claude/plans/eval-parallelism.md§C5.Default behavior
runtime.executor"serial"runtime.fov_workers1runtime.threads_per_worker"auto"cpu_count // fov_workersruntime.cuda_empty_cache_every_n_timepoints0runtime.gc_collect_every_n_fovs0Opt-in process mode example:
runtime.executor=process runtime.fov_workers=4.Plan reference
.claude/plans/eval-parallelism.md(4 review rounds, audit/regression/correctness/merge passes).🤖 Generated with Claude Code